-
Notifications
You must be signed in to change notification settings - Fork 524
[Fix](#649) changes to support accurate ns timepoints #650
[Fix](#649) changes to support accurate ns timepoints #650
Conversation
extensive but not fully tested, unittests updates to follow also fixes ( influxdata#527,influxdata#489, influxdata#346, influxdata#344, influxdata#340)
I'd be happy to merge if it includes unit tests. |
…fBSF/influxdb-python into bug/timestamp_ns_issue_649
Have added fixes to enable passing existing python unit tests. However some failures persist
UNITTESTS
|
While I believe this PR improves the situation, I still see a problem. When trying to write points with microsecond precision, the timestamps are sometimes erroneous (1 us too early) Why? The Solution: Example:
(the assert fails both in current master AND in a version including this PR) |
The changes highlight issue brought up by @tpietruszka requiring precision factor to be of type int as apposed to currently float influxdata#650 (comment)
The changes highlight issue brought up by @tpietruszka requiring precision factor to be of type int as apposed to currently float influxdata#650 (comment)
The changes highlight issue brought up by @tpietruszka requiring precision factor to be of type int as apposed to currently float influxdata#650 (comment)
The issue identified by @tpietruszka #650 (comment) is valid It happens on certain values, I am not well experienced on numpy arithmetic particular corner cases of conversion to challenge what is correct or incorrect. But @tpietruszka code and the simple experiment of test values and different arithmetic strategies for conversion recorded below demonstrate the issue. I have changed the test values in the Unit tests to
This change to unit tests is in the following branch https://github.com/auphofBSF/influxdb-python/tree/bug/microsecond_issue_tpietruszka. The @tpietruszka #650 (comment) result should be identical (There are no NanoSeconds) but where there are nanoseconds what to do ? A QUESTION and DECISION require about Rounding or TruncatingIn the following experiment where there are NanoSecond components of the time should the value be truncated to the MicroSecond or Rounded Up to the Microsecond and for that matter every conversion precision. I am happy to assist with further alterations, thanks to @tpietruszka for identifying and solving to this stage ts = pd.Timestamp('2013-01-01 23:10:55.123456987+00:00')
ts
ts_ns = np.int64(ts.value)
ts_ns
expected_ts_us = 1357081855123456
#as before Pull Request #650
result1 = np.int64(ts_ns / precision_factor)
(expected_ts_us == result1, result1)
# as in Pull Request #650
result2 = np.int64(ts_ns // precision_factor)
(expected_ts_us == result2, result2)
# Tests for Fix identified by @tpietruszka DIVIDE
result3=np.int64(ts_ns / np.int64(precision_factor))
(expected_ts_us == result3, result3)
# Tests for Fix identified by @tpietruszka FLOOR DIVIDE
result4=np.int64(ts_ns // np.int64(precision_factor))
(expected_ts_us == result4, result4) |
from six import iteritems, binary_type, text_type, integer_types, PY2 | ||
|
||
EPOCH = UTC.localize(datetime.utcfromtimestamp(0)) | ||
import pandas as pd # Provide for ns timestamps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add pandas to requirements.txt (and remove pytz and python-dateutil), build is currently failing because of that, see pypy build logs: https://travis-ci.org/influxdata/influxdb-python/jobs/516866320
Great fix btw, looking forward to being merged.
also updated inline docs detail significance of precision factor to be of type int
as identified by @tpietruszka when precision factor is a float, result is an approximation. also define precision factors in 1 place as int's and improved consistency in usage.
7ac0dbf
to
f22ddff
Compare
The commits now include fixes for @tpietruszka identified issues with precision conversion. Test cases have been updated and pass. The only failures still exist with pypy. Following @lipi recommendation in #650 (comment), enabled pypy3 to pass but then failed on all standard python implementations clash of tox.ini deps: and requirements.txt duplicate requirements.
|
#407 seemed much more lightweight to solve the problem. I am not sure why panda is needed just for time conversion, and I do not really understand why all the powers of 10 are defined as floats with the notation 1eX (which result in a float in python) then converted to numpy 64 bits ints. Why not just use 10 ** X which is already a python integer with arbitrary precision? |
This was removed in an upstream PR but reintroduced incorrectly here.
If this still fails |
Duplicate of #407 without |
as documented in (#649).
use floor divided on integer division and use pandas.Timestamp for ns timestamp resolution
Extensively tested but not fully tested, unit test updates to follow
I believe address and fixes ( #527,#489, #346, #344, #340)